Configure New HAProxy to Direct Traffic to Redis with Role: Slave #37
Conversation
rurkss
changed the title
| redisHAProxyName = "redis-haproxy" | ||
| redisSlaveHAProxyName = "redis-s-haproxy" |
There was a problem hiding this comment.
Had you considered putting these in the constants with the other resource naming conventions therein?
Given how other resources the operator names other resource, I'd expect the full generated name of the haproxy bits to be something like:
Redis Failover Redis Master HAproxy
rfrmh-[redis-failover-name]
Redis Failover Redis Master HAproxy
rfrsh-[redis-failover-name]
| func GetHAProxyRedisLabels() map[string]string { | ||
| return haproxyRedisLabels | ||
| } | ||
|
|
||
| func GetHAProxyRedisSlaveLabels() map[string]string { | ||
| return haproxyRedisSlaveLabels | ||
| } |
There was a problem hiding this comment.
Is there a reason we can't reuse the existing generateRedisMasterRoleLabel and generateSlaveRoleLabel functions for this?
redis-operator/operator/redisfailover/service/client.go
Lines 68 to 78 in b3a70fc
|
|
||
| func generateHAProxyDeployment(rf *redisfailoverv1.RedisFailover, labels map[string]string, ownerRefs []metav1.OwnerReference) *appsv1.Deployment { | ||
|
|
||
| name := rf.GenerateName(redisHAProxyName) | ||
| name := rf.GenerateName(labels[rfLabelInstance]) |
There was a problem hiding this comment.
Any reason we don't follow the convention for generating names of resource established in the names.go here?
redis-operator/operator/redisfailover/service/names.go
Lines 17 to 20 in b3a70fc
Had we considered adding a name generators for the HAProxy bits?
| @@ -29,6 +32,8 @@ func (w *RedisFailoverHandler) Ensure(rf *redisfailoverv1.RedisFailover, labels | |||
| } | |||
|
|
|||
| if rf.Spec.Haproxy != nil { | |||
| labels = util.MergeLabels(labels, rfservice.GetHAProxyRedisLabels()) | |||
There was a problem hiding this comment.
This type of thing usually happens down in the generators, no?
| redisRole := "role:master" | ||
| if labels[rfLabelInstance] == redisSlaveHAProxyName { | ||
| redisRole = "role:slave" | ||
| } |
There was a problem hiding this comment.
I see a lot of duplicate branching and inferring based on "role" in the HAProxy resource generators - implicitly in the deployment, and again explicitly in the service generator.
Had you considered exposing explicit functions for creating the master and slave harpoxy bits and levae it up to the caller to decide which one they want; similar to the pattern used the existing Master and Slave service generators?
|
|
||
| var ( | ||
| haproxyRedisSlaveLabels = map[string]string{ | ||
| rfLabelInstance: redisSlaveHAProxyName, |
There was a problem hiding this comment.
Would the existing "redisfailovers.databases.spotahome.com/component" and "redisfailovers.databases.spotahome.com/redisfailovers-role" labels not be enough and/or appropriate here?
AFAIK, the semantics of the app.kuebernetes.io/instance label are that its supposed to unique to the running resource:
Description: A unique name identifying the instance of an application
Example: mysql-abcxzy
-- https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
| Spec: corev1.ServiceSpec{ | ||
| Type: corev1.ServiceTypeClusterIP, | ||
| Selector: map[string]string{ | ||
| "app.kubernetes.io/component": "redis", |
There was a problem hiding this comment.
Low priority context query.
I think this is a "preexisting condition" but any idea why HAProxy components of the RedisFailver would receive the app.kubernetes.io/component label value of "redis" instaed of something like "proxy" or "redis-proxy"?
This Pull Request presents a different method compared to Add Redis Slave Endpoint to HAProxy Service configuration.
Here, the redis-operator will establish a completely new HAProxy service, designed to route traffic specifically to Redis instances functioning as slaves.